Skip to content

fix(backup): use configured imagePullPolicy for backup CronJob#1654

Open
snecklifter wants to merge 1 commit into
devfile:mainfrom
snecklifter:fix/backup-cronjob-imagepullpolicy
Open

fix(backup): use configured imagePullPolicy for backup CronJob#1654
snecklifter wants to merge 1 commit into
devfile:mainfrom
snecklifter:fix/backup-cronjob-imagepullpolicy

Conversation

@snecklifter

@snecklifter snecklifter commented Jun 19, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown

Summary

  • Read imagePullPolicy from DevWorkspaceOperatorConfig for backup Job containers instead of hardcoding "Always"
  • Falls back to "Always" when the field is not explicitly configured, preserving existing default behavior
  • Adds unit tests for both configured and default imagePullPolicy scenarios

Fixes #1637

Test plan

  • Unit test verifies backup Job uses configured imagePullPolicy (e.g. IfNotPresent)
  • Unit test verifies default Always behavior when imagePullPolicy is not set
  • All existing backup controller tests pass (go test ./controllers/backupcronjob/...)
  • go vet clean

Assisted-by: Claude Code

Summary by CodeRabbit

  • New Features

    • Backup operations now respect the workspace's image pull policy configuration, defaulting to "Always" if not specified.

  • Tests

    • Added test coverage validating image pull policy configuration behavior in backup operations.

@snecklifter @claude
fix(backup): use configured imagePullPolicy for backup CronJob
fa59b47 
The backup CronJob controller hardcodes imagePullPolicy to "Always" on
backup Job containers. This does not respect the imagePullPolicy setting
from DevWorkspaceOperatorConfig, unlike other workspace-related pods.

Read imagePullPolicy from the operator config, falling back to "Always"
when the field is not explicitly set.

Fixes: devfile#1637

Assisted-by: Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Chris Brown <chribrow@redhat.com>
@openshift-ci

openshift-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: snecklifter
Once this PR has been reviewed and has the lgtm label, please assign dkwon17 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci

openshift-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

Hi @snecklifter. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4c4bf727-6ccf-4f17-8212-e638b734932d

📥 Commits

Reviewing files that changed from the base of the PR and between cdc8050 and fa59b47.

📒 Files selected for processing (2)
  • controllers/backupcronjob/backupcronjob_controller.go
  • controllers/backupcronjob/backupcronjob_controller_test.go
📝 Walkthrough

Walkthrough

The backup CronJob controller's createBackupJob function previously hard-coded "Always" as the container ImagePullPolicy. A new getImagePullPolicy helper now reads the policy from dwOperatorConfig.Config.Workspace.ImagePullPolicy, defaulting to corev1.PullAlways when unset. Two new tests cover both cases.

Changes

Backup Job ImagePullPolicy from operator config

Layer / File(s) Summary
getImagePullPolicy helper and backup Job wiring
controllers/backupcronjob/backupcronjob_controller.go		 Adds getImagePullPolicy(dwOperatorConfig) that returns dwOperatorConfig.Config.Workspace.ImagePullPolicy when non-empty, otherwise corev1.PullAlways. Updates createBackupJob to use this helper instead of the hard-coded "Always" string.
Tests for configured and default ImagePullPolicy
controllers/backupcronjob/backupcronjob_controller_test.go		 Adds two It blocks: one verifying that IfNotPresent set in operator config is propagated to the generated backup Job's first container, and one verifying that the backup Job defaults to Always when ImagePullPolicy is not configured.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related issues

Suggested labels

lgtm, approved

Suggested reviewers

  • ibuziuk
  • btjd
  • dkwon17
  • rohanKanojia

Poem

🐇 No more "Always" pulling, every single time,
A config check now governs this runtime rhyme.
IfNotPresent skips the registry queue,
The backup Job spec now honors what you configure to.
Hoppy coding, friends — less throttle, more flow! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: using configured imagePullPolicy instead of hardcoding it for the backup CronJob.
Linked Issues check ✅ Passed The PR implements all coding requirements from issue #1637: reads imagePullPolicy from DevWorkspaceOperatorConfig, defaults to Always when unset, and includes unit tests validating both configured and default behaviors.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #1637: modifying backup controller to use configured imagePullPolicy and adding corresponding test coverage with no unrelated alterations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dkwon17 dkwon17 Awaiting requested review from dkwon17 dkwon17 is a code owner

@ibuziuk ibuziuk Awaiting requested review from ibuziuk ibuziuk is a code owner

@akurinnoy akurinnoy Awaiting requested review from akurinnoy akurinnoy is a code owner

@rohanKanojia rohanKanojia Awaiting requested review from rohanKanojia rohanKanojia is a code owner

@btjd btjd Awaiting requested review from btjd btjd is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Backup CronJob should use DevWorkspaceOperatorConfig imagePullPolicy

1 participant

@snecklifter